fix(authz): the connector owner routes require listed membership, not just createdBy (TASK-168) - #1946
Merged
Conversation
… just createdBy (TASK-168) `createdBy` records who made the pod and survives `leavePod`; `members` is what leaving filters. Five owner routes gated on the field alone, so a creator who had left could still read the pod's Discord channel (`GET /:id/messages`) and post into it (`POST /:id/send`) after TASK-161/162 had closed every relay and chat path to them. `canDeleteIntegration`'s pod-creator arm had the same shape, and it is the write gate for six further call sites (ingest tokens, the connect code, PATCH and DELETE). Each pod-creator arm now also requires `isListedPodMember`. No new import: this file already imports the strict rule at line 38, re-exported by `services/connectorRelayPolicy` from `utils/isPodMember` precisely so connector sites read one definition — so there is no second spelling to pick wrong here. The admin and integration-creator arms are untouched: neither ever claimed pod membership. Witnessed per site: eight mutations, one per arm, each reddening exactly its own arm. Nine arms, all real rows on memory Mongo; the two proxy routes assert the Discord call did NOT happen rather than only the status.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cut from
f77c18e5(main after #1943). Backend only, no version bump.Cleared by @vera at
358427ac(74705): each of the six sites reddens its own arm alone, lint 0 errors and 0 on any touched line. Body-only edits since — head unchanged.The defect
createdByrecords who made the pod and survivesleavePod— leaving filtersmembers, not the field. Five owner routes gated on the field alone:POST /:id/connectPOST /:id/disconnectGET /:id/statsGET /:id/messagesDiscordService.fetchMessages)POST /:id/sendDiscordService.sendMessage)After TASK-161/162 closed every relay and chat path to a departed creator, these five still answered to one — the same hole, through a route that calls no predicate at all, which is also why #1940's census walked past them.
canDeleteIntegrationhad the same shape on its pod-creator arm, and that function is the write gate for six further call sites (both ingest-token routes, the connect code, the PATCH, and DELETE).The fix
isListedPodMember(pod, callerId)beside the creator check at each of the five routes, and oncanDeleteIntegration's pod-creator arm. No new import, and no new spelling to pick wrong: this file already imports the strict rule at line 38 —require('../services/connectorRelayPolicy'), which re-exportsutils/isPodMember's strict rule and says why in its own header ("every connector site reads one definition through this module"). The row's spec pointed atutils/isPodMember; that is where the rule is defined, and this file reaches it through the connector layer's re-export. The admin and integration-creator arms are untouched: neither ever claimed pod membership.Why the exposure is exactly "a creator who left"
models/Pod.ts:194pushescreatedByintomembersfor every new pod, so a pod's creator is listed from birth andcreatedBy ∉ membersmeans they left. That is what makes this shape a defect rather than a tautology — and it caught my first version of the arms, which declaredmembers: [bystander]on a creator's pod and passed without ever reaching the predicate: the hook silently repaired the fixture into a listed creator. The fixtures now$pullthe creator after creation, which is whatleavePoddoes and which the hook'sisNewguard leaves alone. A future author adding an arm here will hit the same trap; it is commented in the test file.Exposure count — zero today, measured from both directions
Vera ran it on live Mongo, which my lane cannot reach: 59 pods whose creator is not in
members, carrying 0 integrations, and of the 14 integration rows, 13 have apodIdand 0 of those point at an unlisted creator's pod. Latent, like TASK-164 — the routes are unreachable for the population that exists today, and the two-directional census is what makes that a measurement rather than a hope. (Query asked of Vera: pods wherecreatedByis absent frommembers∩ active integrations, grouped by type.)"A creator no longer listed" — not "a creator who left"
My build note argued that
models/Pod.ts:194(the pre-save hook that pushescreatedByintomemberson every new pod) makescreatedBy ∉ membersmean the creator left. @vera corrected the reason, and it matters for scope:leavePodhas no client caller (wren, 74691), so the 59 pods we measured were not produced by leaving. They were produced by the agent-side removals —agentIdentityService.ts:710andagentInstallationCleanupService.ts:299$pullan agent user, plus three one-shot bot migrations — consistent with all 29 unlisted creators being bots. TheleavePodguard (#1945) bounds the human set at one; the bot set keeps being produced by agent cleanup, guard or no guard. So the guard is right and it is not what protects these five routes: the membership read is, which is why it sits on every pod-creator arm rather than on the leave path.The direction this fix fails in silently
All six sites call
Pod.findById(...)with noselect, andisListedPodMemberneedspod.members— but the TS cast on those lines types the document as{ createdBy?: … }only. A future reader "tightening" that to.select('createdBy')would refuse every creator, listed or not, at all five routes. @vera applied exactly that projection to the messages route and it reddeneda listed creator is unaffected: all five owner routes still act, so the requirement is witnessed by a control rather than by a refusal arm. If you are editing one of those casts, that arm is the one that will tell you.Witnesses
Nine arms, real Pod/Integration/User rows on memory Mongo. The two proxy routes assert the side effect did not happen (
fetchMessages/sendMessagenot called), not only the status — a 403 reached after the call would be no fix at all./:id/connectadded term removedrefuses /connect … and never connects/:id/disconnectterm removedrefuses /disconnect … and never disconnects/:id/statsterm removedrefuses /stats … and never reads stats/:id/messagesterm removedrefuses /messages … and never reads the channel/:id/sendterm removedrefuses /send … and never posts to the channelcanDeleteIntegrationpod arm term removedrefuses DELETE for a pod creator who is no longer listed, and removes nothingcanDeleteIntegrationintegration-creator arm removed (control liveness)the integration-creator arm is untouched: a departed pod creator who created the connector still deletes itEvery mutation ran alone and reddened exactly its named arm — no survivors. Two are controls rather than site mutations and are labelled as such: M7 proves the third arm is what admits the handover case, so my "untouched" claim is witnessed rather than asserted; M8 mutates the shared rule instead of a call site, which is why it reddens six arms at once and is expected to.
Three control arms keep the refusals honest: a listed creator still acts on all five routes (one arm walking all five), a listed pod creator still deletes a connector someone else made, and the integration-creator arm still admits a departed pod creator who created the connector. Without them, "refuse the creator" would pass by refusing every pod that names one.
Runs and lint
__tests__/unit/routes— 140 suites / 1068 tests, green (this adds one suite and nine tests).routes/integrations.ts: 0 eslint errors and 0 warnings on any touched line — intersected eslint's line numbers with the diff's+ranges rather than comparing totals; nofatal: trueanywhere in the run..jscorpus'simport/no-unresolved+import/extensionserrors on its.tsrequires, exactly like every other test file in that corpus. Fixing that corpus is its own task.Gate: Vera.